[RFC] event: expose BOLT12 invoice in PaymentSuccessful for proof of payment #733
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
|
@tnull IDK if this is a good design to have with the ffi, but I had to work around some unify ffi limitation with the enum type that is used inside the |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Exposing the BOLT12 invoice makes sense to me, though we should do it properly instead of just exposing the bytes.
vincenzopalazzo
force-pushed
the
macros/bolt12-pop
branch
from
4c6b18e to
a3a60b3
Compare
There was a problem hiding this comment.
The changes are getting closer, but please make sure to avoid unnecessary boilerplate and stick to the approach we took for the other types (like Offer, Refund, etc).
This also needs a rebase by now, sorry!
Btw, please re-request review when you made updates, otherwise I might not always see it immediately.
bindings/ldk_node.udl
Outdated
| "StaticInvoice", | ||
| }; | ||
|
|
||
| dictionary PaidBolt12Invoice { |
There was a problem hiding this comment.
Can we make this an
[Enum]
interface PaidBolt12Invoice {
instead?
There was a problem hiding this comment.
Mh I was hacking round this this afternoon and this is what I got
error: failed to run custom build command for `ldk-node v0.8.0+git`
Caused by:
process didn't exit successfully (exit status: 101)
--- stdout
cargo:rerun-if-changed=bindings/ldk_node.udl
cargo:rerun-if-env-changed=UNIFFI_TESTS_DISABLE_EXTENSIONS
--- stderr
thread 'main' panicked at build.rs:10:59:
called `Result::unwrap()` on an `Err` value: Objects cannot currently be used in enum variant data
The key part is:
Objects cannot currently be used in enum variant data
This occurs when trying to use:
[Enum]
interface PaidBolt12Invoice {
Bolt12Invoice(Bolt12Invoice invoice);
StaticInvoice(StaticInvoice invoice);
};It looks like that it is happening because Bolt12Invoice and StaticInvoice are defined as interface (Objects) in UniFFI, they cannot be used as parameters in enum variants.
Am I missing something?
There was a problem hiding this comment.
No, at least on our current uniffi version you are indeed correct I'm afraid.
So we are at an impasse here: I really dislike making this a struct with optional fields (and also adding serialization on bindings wrapper, but we could solve for this otherwise). Unfortunately, Uniffi only added support for enum fields that are objects in mozilla/uniffi-rs#2412, which however shipped in v0.29. We however can't update to v0.29 as we have users that use NordSecurity/uniffi-bindgen-go which notoriously lags behind and doesn't get updated (see NordSecurity/uniffi-bindgen-go#40).
So I think this is a rare occasion where I'd be in favor of just not keeping the bindings in sync with the Rust interface for now and coming back to it either when a bindings users shout or once we finally can upgrade.
Could you:
- Revert back to the Rust-only changes, i.e., only using LDK types and putting the
Eventenum field behind#[cfg(not(feature = "uniffi"))] - Add a TODO there in the code noting that we still need to expose this in bindings when we can post uniffi v0.29
- Open a corresponding tracking issue to also keep track of this on github
- Rebase and squash any fixup commits so we can land this soon
Sorry for the churn, hope you didn't spend too much time trying to make it work!
There was a problem hiding this comment.
No problem, thanks for helping navigate UniFFI. So I did what you suggested in this commit: e6200d6. But to me, this solution still feels a little bit incomplete because I think we can do better and remove the branching inside UniFFI.
Do you think it's worth reconsidering my first approach to pass the raw bytes of the invoice? That way, people using the bindings can still perform proof of payment, and then when the new version of UniFFI lands, we can deprecate the raw bytes and switch to the proper type?
let me know what do you preferer, the PR to be ready need just to squash the last commit
vincenzopalazzo
force-pushed
the
macros/bolt12-pop
branch
4 times, most recently
from
22abd3b to
a8d05cb
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
vincenzopalazzo
force-pushed
the
macros/bolt12-pop
branch
2 times, most recently
from
78d71dc to
327a59a
Compare
After a successful BOLT12 payment, users need access to the paid invoice to provide proof of payment to third parties. The invoice contains the payment_hash that can be verified against sha256(preimage). This adds a `bolt12_invoice` field to `PaymentSuccessful` containing the `PaidBolt12Invoice` which wraps either a regular `Bolt12Invoice` or a `StaticInvoice` for async payments. For UniFFI bindings, `PaidBolt12Invoice` is exposed as a dictionary struct that delegates serialization to LDK's native type. For non-UniFFI builds, we simply re-export LDK's `PaidBolt12Invoice` enum directly. Signed-off-by: Vincenzo Palazzo <[email protected]>
Due to UniFFI 0.28 limitations (Object types in enum variants are not supported), the `bolt12_invoice` field in `PaymentSuccessful` cannot be exposed in bindings until we upgrade to UniFFI 0.29+. Problem: The previous commit added `PaidBolt12Invoice` to the `PaymentSuccessful` event, but UniFFI 0.28 cannot handle Object types (like `Bolt12Invoice`) inside enum variant data, causing binding generation to fail. Solution: Use `#[cfg(not(feature = "uniffi"))]` to conditionally compile the `bolt12_invoice` field only for non-UniFFI builds. This requires duplicating the `impl_writeable_tlv_based_enum!` macro invocation for Event, but the serialization formats remain compatible: TLV tag 7 written by non-UniFFI builds is silently skipped by UniFFI builds (unknown optional TLV), and vice versa. Changes: - Add `#[cfg]` guards to `bolt12_invoice` field in `PaymentSuccessful` - Duplicate Event serialization macro for UniFFI/non-UniFFI builds - Remove unused `StaticInvoice` and `PaidBolt12Invoice` FFI wrappers - Add consistent TODO comments referencing lightningdevkit#757 for tracking - Update tests to handle both build configurations
vincenzopalazzo
force-pushed
the
macros/bolt12-pop
branch
from
327a59a to
e6200d6
Compare
3 participants
This patch adds the
bolt12_invoicefield to thePaymentSuccessfulevent, enabling users to obtain proof of payment for BOLT12 transactions.Problem:
Previously, after a successful BOLT12 payment, users had no way to access the paid invoice data. This made it impossible to provide proof of payment to third parties, who need both the payment preimage and the original invoice to verify that sha256(preimage) matches the invoice's payment_hash.
Solution:
Add a
bolt12_invoice: Option<Vec<u8>>field toPaymentSuccessfulthat contains the serialized BOLT12 invoice bytes. The invoice is serialized using LDK's standard encoding, which can be parsed back usingBolt12Invoice::try_from(bytes)in native Rust, or by hex-encoding the bytes and usingBolt12Invoice.from_str()in FFI bindings.Design decisions:
Vec<u8>rather than the complexPaidBolt12Invoicetype to avoid UniFFI limitations with objects in enum variantsNoneforStaticInvoice(async payments) since proof of payment is not possible for those payment types anywayThis implementation follows the maintainer guidance from PR #563 to expose the invoice via the event rather than storing it in the payment store.